Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add keyword arguments to IOBuffer's constructors #25872

Merged
merged 5 commits into from
Feb 6, 2018

Conversation

bicycle1885
Copy link
Member

@bicycle1885 bicycle1885 commented Feb 4, 2018

This adds constructors of IOBuffer taking keyword arguments (e.g. write=true) as open does, which will supersede #24430.

TODO:

  • Update docs.
  • Deprecate old constructors that accept positional arguments?
  • Update constructor calls if we deprecate positional arguments.

@bicycle1885
Copy link
Member Author

I think this is ready to review. Please ignore the last commit, which just removes a junk file I made accidentally.

I'm still wondering whether we should choose IOBuffer(write=true) or IOBuffer(read=true, write=true, truncate=true) by default. I think the former is preferable since it is minimal and consistent to open but it is breaking because it makes IOBuffer() no longer readable by default:

julia> Base.open_flags(write=true)
(read = false, write = true, create = true, truncate = true, append = false)

julia> Base.open_flags(read=true, write=true, truncate=true)
(read = true, write = true, create = true, truncate = true, append = false)

Any comments? @vtjnash @StefanKarpinski

base/iobuffer.jl Outdated
- `truncate`: truncates the buffer size to zero length.
- `maxsize`: specifies a size beyond which the buffer may not be grown.

When `data` is given, the buffer will be both readable and writable by default.
Copy link
Member

@JeffBezanson JeffBezanson Feb 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is true --- the keyword arguments default to nothing, which open_flags interprets as read-only. And that's probably a good thing; for example we have

IOBuffer(str::String) = IOBuffer(unsafe_wrap(Vector{UInt8}, str))

which we want to remain read-only.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right. I meant "When data is not given, ...". Fixed.


julia> String(take!(io))
"JuliaLang is a GitHub organization. It has many members."

julia> io = IOBuffer("JuliaLang is a GitHub organization.")
IOBuffer(data=UInt8[...], readable=true, writable=false, seekable=true, append=false, size=35, maxsize=Inf, ptr=1, mark=-1)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The examples don't include the truncate flag. As an aside, I guess I should write the code to figure out a minimal set of keywords to reproduce a particular set of open flags – this output is a bit on the long side.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. I added some examples in #25919.

@JeffBezanson JeffBezanson merged commit 8076a1b into JuliaLang:master Feb 6, 2018
maxsize=maxsize)
buf.data[:] = 0
if flags.truncate
buf.size = 0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not important, just wondering: since truncate is also passed to the constructor above, is this redundant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it is. Fixed in #25919.

@bicycle1885 bicycle1885 deleted the iobuffer-kwargs branch February 7, 2018 01:39
@@ -512,7 +512,7 @@ isascii(s::AbstractString) = all(isascii, s)
## string map, filter, has ##

function map(f, s::AbstractString)
out = IOBuffer(StringVector(sizeof(s)), true, true)
out = IOBuffer(StringVector(sizeof(s)), read=true, write=true)
Copy link
Member

@stevengj stevengj Feb 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing a StringVector like this seems to be pretty common. It would be good to add a sizehint keyword argument so that you could just do IOBuffer(sizehint=sizeof(s)) here and in similar places.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I'll take care of it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants